Code Actions - Add fix all support #6310
Conversation
|
Thank you for this, great work! 🎉 |
| resolveCodeAction: async (codeAction, token, next) => { | ||
| const lspCodeAction = <CodeAction>codeAction; | ||
| const data = lspCodeAction.data; | ||
| if (data.FixAllFlavors) { |
There was a problem hiding this comment.
I know it comes from Roslyn, but it is amusing that it is called FixAll"Flavors" and not "Scopes" or "Kinds".
| @@ -202,6 +205,46 @@ export class RoslynLanguageServer { | |||
| workspace: { | |||
| configuration: (params) => readConfigurations(params), | |||
| }, | |||
| resolveCodeAction: async (codeAction, token, next) => { | |||
There was a problem hiding this comment.
So I like the approach you took here, but it is slightly different from what we talked about before. I like this better anyway though, but we can simplify a little bit more.
Since you're directly applying the workspace edit (via vscode.workspace.applyEdit) and not returning a resolved code action to vscode, it would be better to have this implementation just be a command.
Essentially:
- On the server side, instead of adding the flavor to the 'data' object, you would return a fix all code action that contains a command (e.g.
roslyn.fixAll) with arguments containing whatever flavor information you need to call back to the server as you do now. An example of where we do this is here - https://github.com/dotnet/roslyn/blob/main/src/Features/LanguageServer/Protocol/Handler/CodeLens/CodeLensHandler.cs#L149 - Instead of implementing this as part of the
resolveCodeActionmiddleware - you would move this implementation to a command handler. The implementation here basically wouldn't change (minus my other feedback). For example, where the client implements the command I linked above - https://github.com/dotnet/vscode-csharp/blob/main/src/lsptoolshost/unitTesting.ts#L21
Then what happens is that when you select the fix all action in the list - the vscode LSP client will invoke our command handler (based on the command id we returned in the code action)
| const result = await protocolConverter.asWorkspaceEdit(response.edit); | ||
|
|
||
| if (!(await vscode.workspace.applyEdit(result))) { | ||
| throw new Error( |
There was a problem hiding this comment.
after you've finished addressing the other comments (e.g. moving to a command), can you make sure that if this fails it shows an error toast? if it doesn't we might need to explicitly show one.
| @@ -202,6 +203,46 @@ export class RoslynLanguageServer { | |||
| workspace: { | |||
| configuration: (params) => readConfigurations(params), | |||
| }, | |||
| // resolveCodeAction: async (codeAction, token, next) => { | |||
src/lsptoolshost/fixAllCodeAction.ts
Outdated
| } | ||
| ); | ||
|
|
||
| if (!(await vscode.workspace.applyEdit(fixAllEdit))) { |
There was a problem hiding this comment.
I believe the apply edit should go inside the progress as well. I think it won't be cancellable because you don't pass in the cancellation token
src/lsptoolshost/fixAllCodeAction.ts
Outdated
| ); | ||
|
|
||
| if (!(await vscode.workspace.applyEdit(fixAllEdit))) { | ||
| throw new Error('Tried to insert multiple code action edits, but an error occurred.'); |
There was a problem hiding this comment.
test this to make sure the error is visible (hopefully in the C# output window). If not you may need to manually write it there via passing in the output channel
CodeActiontype,RoslynFixAllCodeActionthat specifies the scope being used for fix allCodeActiontype that contains aWorkspaceEditwith fix all editsWorkspaceEditto a VS CodeWorkspaceEditand tries to apply it.Untitled.video.mp4